Skip to content

Add tests for incremental payloads with custom scalars - #13319

Merged
jerelmiller merged 17 commits into
release-4.3from
jerel/more-scalar-tests
Jul 13, 2026
Merged

Add tests for incremental payloads with custom scalars#13319
jerelmiller merged 17 commits into
release-4.3from
jerel/more-scalar-tests

Conversation

@jerelmiller

@jerelmiller jerelmiller commented Jul 7, 2026

Copy link
Copy Markdown
Member

Adds tests to ensure scalar values are applied to incremental payloads (both @defer and @stream).

Summary by CodeRabbit

  • Bug Fixes

    • Improved consistency when handling custom scalar values, including dates, in queries, mutations, subscriptions, and React hooks.
    • Custom scalar data is now correctly parsed and preserved across GraphQL errors and different error policies.
    • Improved handling of custom scalars during deferred and streamed responses.
    • Preserved result identity when refreshed data is unchanged, helping avoid unnecessary updates.
  • Tests

    • Expanded coverage across loading, refetching, partial-data, and error scenarios.

@jerelmiller
jerelmiller requested review from DaleSeo and phryneas July 7, 2026 23:29
@apollo-librarian

apollo-librarian Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: c95e9f8f66e97c603022292c
Build Logs: View logs


✅ AI Style Review — No Changes Detected

No MDX files were changed in this pull request.

Review Log: View detailed log

This review is AI-generated. Please use common sense when accepting these suggestions, as they may not always be accurate or appropriate for your specific context.

@changeset-bot

changeset-bot Bot commented Jul 7, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 832690e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Custom scalar behavior

Layer / File(s) Summary
Core client operation coverage
src/core/__tests__/client.*/customScalars.test.ts
Adds Date scalar identity, error-policy, subscription, mutation, query, watch-query, and incremental delivery coverage.
Query, lazy-query, and mutation hooks
src/react/hooks/__tests__/useQuery/*, useLazyQuery/*, useMutation/*
Tests scalar identity, error results, and deferred or streamed scalar parsing.
Suspense and background query coverage
src/react/hooks/__tests__/useBackgroundQuery/*, useSuspenseQuery/*, useQueryRefHandlers/*
Adds render-flow, refetch identity, error-policy, and incremental delivery assertions.
Loadable query harness and preloader coverage
src/react/hooks/__tests__/useLoadableQuery/*, src/react/query-preloader/*, config/jest.config.ts
Adds a shared loadable-query test harness and expands preloader and loadable scalar tests across policies and incremental handlers.

Estimated code review effort: 5 (Critical) | ~90 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: phryneas, daleseo, phryneas

Poem

A rabbit hops through Dates so bright,
Parsing errors left and right.
Refetches keep the same old view,
Streams bring scalar fields anew.
“Defer!” cries Bun, then tests take flight—
The cache now thumps with typed delight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: new tests for incremental payloads with custom scalars.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pkg-pr-new

pkg-pr-new Bot commented Jul 7, 2026

Copy link
Copy Markdown
npm i https://pkg.pr.new/@apollo/client@13319

commit: 832690e

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
src/core/__tests__/client.query/customScalars.test.ts (1)

568-639: 🎯 Functional Correctness | 🔵 Trivial

Unresolved TODO baked into assertion.

The test asserts startDate is parsed inside CombinedGraphQLErrors.data with a // TODO: Determine if this is correct comment. This pattern (serialized in none/ignore policies vs. parsed here) repeats identically across client.mutate, client.subscribe, and createQueryPreloader tests. Worth resolving whether error.data should stay serialized (matching the none policy test) or intentionally diverge for all.

Want me to open a tracking issue for this?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/__tests__/client.query/customScalars.test.ts` around lines 568 -
639, The test in customScalars.test.ts contains an unresolved TODO inside the
CombinedGraphQLErrors.data assertion for client.query with errorPolicy "all".
Remove the TODO and make the expectation consistent with the intended behavior
for scalar handling in error data: either assert the serialized value to match
the "none"/"ignore" cases, or keep the parsed Date only if that is the
deliberate contract for errorPolicy "all". Apply the same decision consistently
across the related client.mutate, client.subscribe, and createQueryPreloader
tests so the behavior is uniform.
src/core/QueryInfo.ts (1)

329-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Correct fix, matches diff.result nullability contract.

The broadened condition correctly writes back the in-flight incremental diff so parsed scalars propagate to intermediate @defer chunks. Since diff.result is typed as DeepPartial<T> | null (never undefined) when !diff.complete, the !== null check is safe.

One minor nit: this.incremental?.hasNext duplicates the existing hasNext getter (Line 182-184) which already encapsulates this exact check. Using this.hasNext here would be slightly more consistent with usage elsewhere in the file (e.g., Lines 405, 513, 526).

♻️ Optional consistency tweak
           if (
             diff.complete ||
             // Use the diff result if the query is still an in-flight
             // incremental query so that parsed custom scalar values
             // in diff.result are returned.
-            (this.incremental?.hasNext && diff.result !== null)
+            (this.hasNext && diff.result !== null)
           ) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/QueryInfo.ts` around lines 329 - 337, The incremental diff
write-back in QueryInfo should use the existing hasNext getter instead of
duplicating its logic with this.incremental?.hasNext. Update the condition in
the result merge block so it checks this.hasNext together with diff.result !==
null, keeping the current nullability-safe behavior while matching the rest of
QueryInfo’s usage and reducing duplicated state checks.
src/core/__tests__/client.watchQuery/customScalars.test.ts (1)

1105-1193: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Unresolved TODO questions correctness of an assertion.

// TODO: Determine if this is correct (line 1173-1174) flags uncertainty about whether the parsed startDate should appear inside CombinedGraphQLErrors.data for errorPolicy: "all". This same TODO is duplicated across several other new test files in this PR (useQuery, useLazyQuery, useMutation, useBackgroundQuery, useSuspenseQuery, useLoadableQuery). Worth resolving before merge since it signals the author isn't fully confident in the asserted behavior.

Want me to help investigate the expected error-data shape for errorPolicy: "all" and propose a fix/clarifying comment?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/__tests__/client.watchQuery/customScalars.test.ts` around lines 1105
- 1193, The test assertion in ObservableStream for `client.watchQuery` has an
unresolved TODO about the shape of `CombinedGraphQLErrors.data` under
`errorPolicy: "all"`. Confirm the expected error payload for parsed scalars in
the `watchQuery` path and then either keep the parsed `startDate` in the
asserted error data or adjust the assertion to match the actual behavior,
removing the TODO; also apply the same fix pattern to the matching new tests in
the other query/mutation hooks if they share this uncertainty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/core/__tests__/client.query/customScalars.test.ts`:
- Around line 568-639: The test in customScalars.test.ts contains an unresolved
TODO inside the CombinedGraphQLErrors.data assertion for client.query with
errorPolicy "all". Remove the TODO and make the expectation consistent with the
intended behavior for scalar handling in error data: either assert the
serialized value to match the "none"/"ignore" cases, or keep the parsed Date
only if that is the deliberate contract for errorPolicy "all". Apply the same
decision consistently across the related client.mutate, client.subscribe, and
createQueryPreloader tests so the behavior is uniform.

In `@src/core/__tests__/client.watchQuery/customScalars.test.ts`:
- Around line 1105-1193: The test assertion in ObservableStream for
`client.watchQuery` has an unresolved TODO about the shape of
`CombinedGraphQLErrors.data` under `errorPolicy: "all"`. Confirm the expected
error payload for parsed scalars in the `watchQuery` path and then either keep
the parsed `startDate` in the asserted error data or adjust the assertion to
match the actual behavior, removing the TODO; also apply the same fix pattern to
the matching new tests in the other query/mutation hooks if they share this
uncertainty.

In `@src/core/QueryInfo.ts`:
- Around line 329-337: The incremental diff write-back in QueryInfo should use
the existing hasNext getter instead of duplicating its logic with
this.incremental?.hasNext. Update the condition in the result merge block so it
checks this.hasNext together with diff.result !== null, keeping the current
nullability-safe behavior while matching the rest of QueryInfo’s usage and
reducing duplicated state checks.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 17cce7bb-f976-4338-9293-6d15981fdbdd

📥 Commits

Reviewing files that changed from the base of the PR and between 8ab63fc and 5dd99b9.

📒 Files selected for processing (15)
  • .changeset/nasty-cheetahs-confess.md
  • src/core/QueryInfo.ts
  • src/core/__tests__/client.mutate/customScalars.test.ts
  • src/core/__tests__/client.query/customScalars.test.ts
  • src/core/__tests__/client.readQuery/customScalars.test.ts
  • src/core/__tests__/client.subscribe/customScalars.test.ts
  • src/core/__tests__/client.watchQuery/customScalars.test.ts
  • src/react/hooks/__tests__/useBackgroundQuery/customScalars.test.tsx
  • src/react/hooks/__tests__/useLazyQuery/customScalars.test.tsx
  • src/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsx
  • src/react/hooks/__tests__/useMutation/customScalars.test.tsx
  • src/react/hooks/__tests__/useQuery/customScalars.test.tsx
  • src/react/hooks/__tests__/useQueryRefHandlers/customScalars.test.tsx
  • src/react/hooks/__tests__/useSuspenseQuery/customScalars.test.tsx
  • src/react/query-preloader/__tests__/createQueryPreloader/customScalars.test.tsx

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has a larger diff because I created a render helper and updated the existing tests to use it. Sorry for the extra noise!

event: {
__typename: "Event",
id: "1",
startDate: "2026-01-01",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to flush out what we want this value to be. In this test, the value is the raw network value because the value was never written to the cache (because of the error policy), so the value is never parsed into a Date object. In the errorPolicy: "all" test below, the startDate is the parsed Date object in this error object because markQueryResult in QueryInfo writes the value to the cache and re-read with the parsed value before the errorPolicy is handled in QueryManager.

The benefit to doing it that way is that CombinedGraphQLErrors sets data as the combined result for incremental payloads rather than just the current chunk, however it does mean the inconsistency in the data value. Is this ok?

@jerelmiller

Copy link
Copy Markdown
Member Author

Moving to draft because I need some further discussion on the failing tests.

@jerelmiller
jerelmiller marked this pull request as draft July 8, 2026 04:50
@jerelmiller

Copy link
Copy Markdown
Member Author

Much of this should be fixed with #13324. Once that is merged, I'll put this back up for review which should be mostly tests at that point.

Comment thread src/core/QueryInfo.ts Outdated
@github-actions github-actions Bot added the auto-cleanup 🤖 label Jul 13, 2026
@jerelmiller
jerelmiller force-pushed the jerel/more-scalar-tests branch from 32599de to 0ecf5e1 Compare July 13, 2026 22:56
@jerelmiller jerelmiller changed the title Fix custom scalars not returning parsed value in intermediate @defer payloads Add tests for incremental payloads with custom scalars Jul 13, 2026
@jerelmiller

Copy link
Copy Markdown
Member Author

@coderabbitai rereview

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

@jerelmiller Sounds good, I'll take another look at the PR now that the changes are in place.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@jerelmiller
jerelmiller marked this pull request as ready for review July 13, 2026 23:22

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsx (1)

541-558: 🩺 Stability & Availability | 🔵 Trivial

Resolve the TODO on error-data scalar serialization before un-drafting.

Under errorPolicy: "all" the CombinedGraphQLErrors data expects startDate as a parsed Date, whereas the errorPolicy: "none" test (Line 443) expects the raw string "2026-01-01" for the same field. This asymmetry is exactly what the // TODO: Determine if this is correct comment questions and should be pinned down so the test encodes intended behavior rather than incidental behavior.

Want me to open an issue to track deciding whether error-embedded scalars should be parsed or left serialized?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsx` around
lines 541 - 558, Resolve the TODO in the error-policy test by establishing the
intended scalar representation for error-embedded data and making the
`errorPolicy: "all"` and `errorPolicy: "none"` assertions consistent. Update the
test data and expectations around `CombinedGraphQLErrors` and the corresponding
`startDate` case so the chosen parsed-`Date` or raw-string behavior is
explicitly pinned down.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsx`:
- Around line 541-558: Resolve the TODO in the error-policy test by establishing
the intended scalar representation for error-embedded data and making the
`errorPolicy: "all"` and `errorPolicy: "none"` assertions consistent. Update the
test data and expectations around `CombinedGraphQLErrors` and the corresponding
`startDate` case so the chosen parsed-`Date` or raw-string behavior is
explicitly pinned down.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: da00ade5-2c3c-4cda-9969-6cecea52b939

📥 Commits

Reviewing files that changed from the base of the PR and between 5dd99b9 and 832690e.

📒 Files selected for processing (17)
  • config/jest.config.ts
  • src/core/__tests__/client.mutate/customScalars.test.ts
  • src/core/__tests__/client.query/customScalars.test.ts
  • src/core/__tests__/client.readQuery/customScalars.test.ts
  • src/core/__tests__/client.subscribe/customScalars.test.ts
  • src/core/__tests__/client.watchQuery/customScalars.test.ts
  • src/react/hooks/__tests__/useBackgroundQuery/customScalars.test.tsx
  • src/react/hooks/__tests__/useBackgroundQuery/testUtils.tsx
  • src/react/hooks/__tests__/useLazyQuery/customScalars.test.tsx
  • src/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsx
  • src/react/hooks/__tests__/useLoadableQuery/testUtils.tsx
  • src/react/hooks/__tests__/useMutation/customScalars.test.tsx
  • src/react/hooks/__tests__/useQuery/customScalars.test.tsx
  • src/react/hooks/__tests__/useQueryRefHandlers/customScalars.test.tsx
  • src/react/hooks/__tests__/useSuspenseQuery/customScalars.test.tsx
  • src/react/hooks/__tests__/useSuspenseQuery/testUtils.tsx
  • src/react/query-preloader/__tests__/createQueryPreloader/customScalars.test.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
  • src/core/tests/client.subscribe/customScalars.test.ts
  • src/react/query-preloader/tests/createQueryPreloader/customScalars.test.tsx
  • src/react/hooks/tests/useLazyQuery/customScalars.test.tsx
  • src/react/hooks/tests/useQuery/customScalars.test.tsx
  • src/react/hooks/tests/useBackgroundQuery/customScalars.test.tsx
  • src/core/tests/client.query/customScalars.test.ts
  • src/react/hooks/tests/useSuspenseQuery/customScalars.test.tsx
  • src/core/tests/client.mutate/customScalars.test.ts
  • src/react/hooks/tests/useMutation/customScalars.test.tsx
  • src/react/hooks/tests/useQueryRefHandlers/customScalars.test.tsx
  • src/core/tests/client.watchQuery/customScalars.test.ts

@jerelmiller

Copy link
Copy Markdown
Member Author

As @coderabbitai notes, we haven't resolved the TODO comment in the tests. I'd like to get another alpha out with these fixes in place, so we will revisit this once again before rc.

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@jerelmiller Understood — thanks for the update. Sounds like a reasonable plan to ship the alpha now and circle back on the TODO before the rc release. I'll keep an eye out and flag it again if it's still outstanding when this comes back up for review.

(・ω・)ノ

@jerelmiller
jerelmiller merged commit 8c37250 into release-4.3 Jul 13, 2026
53 checks passed
@jerelmiller
jerelmiller deleted the jerel/more-scalar-tests branch July 13, 2026 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants